Merge TraceLayout into TraceInfo
#245
Conversation
There was a problem hiding this comment.
Thank you! Looks good! I left some comments inline - but nothing major.
The original motivation for having TraceInfo and TraceLayout be separate was that TraceLayout was supposed to be independent from a specific instance of a computation (i.e., would not depend on trace length). But I don't think this is actually needed in practice - so, merging them is a good improvement.
| aux_segment_widths: [usize; NUM_AUX_SEGMENTS], | ||
| aux_segment_rands: [usize; NUM_AUX_SEGMENTS], |
There was a problem hiding this comment.
Comment for the future: if we assume that there will only be a single auxiliary trace segment, we can simplify these to be primitive values rather than arrays.
prover/src/trace/mod.rs
Outdated
| /// Returns the number of columns in the main segment of this trace. | ||
| fn main_trace_width(&self) -> usize { | ||
| self.layout().main_trace_width() | ||
| } | ||
|
|
||
| /// Returns the number of columns in all auxiliary trace segments. | ||
| fn aux_trace_width(&self) -> usize { | ||
| self.layout().aux_trace_width() | ||
| } |
There was a problem hiding this comment.
Similar comment to the one above: maybe it is worth keeping these as separate methods?
There was a problem hiding this comment.
Reintroduced them too.
For the future though, I find selectively introducing passthrough methods for some of the TraceInfo method and not others can add confusion. For example, instead of knowing that all trace parameters are in the TraceInfo, accessible through Trace::info(), I have to remember/double-check if that one method I has a passthrough. This was the original motivation for removing all passthroughs.
This is more of a nit/personal preference though.
|
@irakliyk should we fix the new redundant imports clippy errors in another PR? There are a lot of them (more than the CI shows), and it might require a change in how |
math/src/field/traits.rs
Outdated
| fn from_byte_vec_with_padding(mut bytes: Vec<u8>) -> Self { | ||
| assert!(bytes.len() < Self::ELEMENT_BYTES); | ||
|
|
||
| /// Returns a canonical integer representation of this field element. | ||
| fn as_int(&self) -> Self::PositiveInteger; | ||
| bytes.resize(Self::ELEMENT_BYTES, 0); | ||
|
|
||
| match Self::try_from(bytes.as_slice()) { | ||
| Ok(element) => element, | ||
| Err(_) => panic!("element deserialization failed"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential optimization: I wonder if we should change the function signature to:
fn from_bytes_with_padding(bytes: &[u8]) -> SelfThis would avoid the need to call to_vec() for the users of this function which could save on heap allocations. Internally, we could use an array buffer like so:
let mut buf = [0; Self::ELEMENT_BYTES];
buf[..bytes.len()].copy_from_slice(bytes);There was a problem hiding this comment.
This gives me this error:
error: constant expression depends on a generic parameter
--> math/src/field/traits.rs:286:27
|
286 | let mut buf = [0; Self::ELEMENT_BYTES];
| ^^^^^^^^^^^^^^^^^^^
|
= note: this may fail depending on what value the parameter takes
I did a little digging, and this seems to be a part of Rust that is still rough around the edges. IIUC, the problem is currently that there is no syntax to write the trait bounds for Self::ELEMENT_BYTES that the compiler needs to ensure soundness.
There was a problem hiding this comment.
I would probably still change the method signature and do .to_vec() internally. This way, when we can do it properly in the future, we can make the change only in one place.
There was a problem hiding this comment.
fixed.
Note that the let element = ...; variable was necessary to avoid the compiler complaining about lifetimes.
Yes - let's address these in a separate PR. |
|
@irakliyk everything should be good to go now |
This is in preparation for #240.
The initial reason for this PR was to prepare for the introduction of the Lagrange kernel. Specifically,
TraceLayoutstored the number of random elementsaux_segment_randsneeded by the auxiliary trace; with the Lagrange kernel transition constraint, this number is a function of the trace length (specificallylog(trace_length)). So the trace length should be stored in the same struct asaux_segment_rands.Ultimately I found that the cleanest solution was to collapse the
TraceLayoutinto theTraceInfo. As an aside, a "trace layout" without all the layout information (i.e. missing trace length) didn't make sense to me. I also found that this change cleaned things up, specifically sinceTraceInfowas stored "in parts" in multiple places (new to aTraceLayout) - we now properly store a singleTraceInfoobject in these cases.This change also remove a bunch of methods across the board, which in my opinion simplifies the general API. For example, the only way to get a trace length now is through a
TraceInfoobject (unless I missed some), and all traces return theirTraceInfo.